refactor(web): DRY orderedProfiles, fix circular imports, consolidate Profile type (LUM-2072)#33121
Conversation
… Profile type (LUM-2072)
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…, dead export, duplicate mutations - manage-profiles-modal: pass orderedProfiles from hook instead of recomputing via buildOrderedProfiles - call-site-overrides-modal: remove unnecessary CUSTOM_SENTINEL export (only used within file) - use-daemon-config: eliminate duplicate patchConfigMutation and putImageGenModelMutation useMutation hooks — patchDaemonConfig and setImageGenModelOnDaemon now call SDK directly with finally-based cache invalidation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ Vex Review — APPROVE
Quick-wins audit cleanup on domains/settings/ai/ from the LUM-2072 first-principles pass. Five categories, all mechanical, all earn their place.
Verification at HEAD 0d006e35 (post-merge)
Conflict resolved against main (which gained #33094 + #33113 since this branch was cut). New HEAD is a merge commit — re-verified the audit-commit semantics survived intact through the merge.
Codex 👍 at HEAD. Boss triggered 22:15 against 0d006e35; Codex 22:18: "Didn't find any major issues. Keep them coming!" + 👍. Pre-trigger gate held. Devin's "No Issues Found" was at sha 1 (predates audit + merge), but Codex covered the post-merge HEAD.
Five Fixes, All Earned
1. buildOrderedProfiles extracted (ai-utils.ts:24). Single pure function replaces three copies of "merge profileOrder with extras not in order" across language-model-card, call-site-overrides-modal, manage-profiles-modal. Doc-commented for stability semantics. useDaemonConfig exposes the derived slice via useMemo([profiles, profileOrder]) — N consumers now share one memoization, not three.
2. Circular import broken (ai-types.ts:7). CallSiteOverrideDraft moved out of call-site-overrides-modal.tsx (a component) into ai-types.ts (the types module). Was previously: component file owned the type → ai-types.ts imported from it → use-daemon-config.ts imported from ai-types. Types-from-components is exactly the cycle vector this fixes.
3. Redundant Profile type + mapper deleted (manage-profiles-modal.tsx −35 lines). Replaced with ProfileWithName = { name: string } & ProfileEntry intersection in ai-types.ts. Mapper profileEntryToProfile (manual field copy across 12 fields) gone — intersection types do this structurally. Cross-component type dependency from profile-editor-modal → manage-profiles-modal removed.
4. Incomplete migration tightened. manage-profiles-modal was calling buildOrderedProfiles locally instead of consuming the orderedProfiles slice the hook now exposes. Dead export on CUSTOM_SENTINEL (only used within file) removed. Mechanical hygiene the LUM-2072 follow-up arc was specifically auditing for.
5. Duplicate mutation wrappers eliminated (use-daemon-config.ts −30 lines). The hook previously defined patchConfigMutation and putImageGenModelMutation via useMutation, structurally identical to the separately-exported useDaemonConfigMutation(). The convenience wrappers (patchDaemonConfig, setImageGenModelOnDaemon) already owned error-toast + lazy ID resolution + Sentry capture. The useMutation wrappers only added onSettled invalidation — converted to finally { invalidateConfig(); } blocks. Verified at HEAD use-daemon-config.ts:128-150 + :152-174 — both functions resolve assistantId, call the SDK directly (configPatch/modelImagegenPut), invalidate in finally. The useMutation indirection wasn't earning its keep here; SDK-direct + finally is cleaner and removes the mutation-state surface the callers weren't reading.
Anti-pattern Grep
Diff scanned line-by-line for as casts, non-null assertions, || 0 fallbacks: zero hits. The only ! in the diff is profiles[name]! inside buildOrderedProfiles — justified by the .filter((name) => name in profiles) guard directly above. TS's in-narrowing doesn't carry through .map, so the assertion is correct.
Follow-up Tickets Filed (LUM-2072 sub-issues)
- LUM-2184: Split
useDaemonConfiggod-hook into query hook + separate mutation helpers - LUM-2185: Add type safety to daemon config mutation body (replace
Record<string, unknown>) - LUM-2186: Replace one-shot
useEffecthydration with query-derived draft state in service cards
Good triage — these are the right separations to defer; useDaemonConfig is already doing more than the first refactor scoped for, and bundling the split into this PR would obscure the quick-wins. The audit commit's Record<string, unknown> mutation body (LUM-2185) is the only typing seam I'd push to land sooner.
Net
+76 / −155 across 7 files. Three architectural anti-patterns closed (duplicate derivation, circular import, redundant type), two cleanups (dead export, duplicate useMutation), zero behavior change, three sub-tickets filed for the deferred work. Test plan green. CI 7/7 green.
Ship it.
1 participant
Why
Five architectural issues in the settings/ai domain identified during the LUM-2072 first-principles audit:
Duplicated ordering logic — The same "merge
profileOrderwith extras not in order" derivation was copy-pasted acrosslanguage-model-card.tsx,call-site-overrides-modal.tsx, andmanage-profiles-modal.tsx. A single utility function eliminates the duplication and ensures ordering semantics stay consistent.Circular import —
CallSiteOverrideDraftwas defined incall-site-overrides-modal.tsx(a component file), then imported byai-types.tsanduse-daemon-config.ts. Types should live in the types module, not in a component that then becomes an implicit dependency for unrelated modules.Redundant type + mapper —
manage-profiles-modal.tsxdefined aProfileinterface that was identical to{ name: string } & ProfileEntry, plus aprofileEntryToProfilemapper that manually copied every field. The separate type was imported byprofile-editor-modal.tsx, creating a cross-component type dependency for no reason.Incomplete consumer migration — After adding
orderedProfilesto the hook,manage-profiles-modal.tsxstill calledbuildOrderedProfileslocally instead of using the precomputed value. Dead exportCUSTOM_SENTINELincall-site-overrides-modal.tsxwas exported but never imported by any other file.Duplicate mutation instances —
useDaemonConfig()defined its ownpatchConfigMutationandputImageGenModelMutationviauseMutation, structurally identical to the separately-exporteduseDaemonConfigMutation(). The convenience wrappers (patchDaemonConfig,setImageGenModelOnDaemon) already handle error toasts, lazy ID resolution, and Sentry capture — the internaluseMutationwrappers only addedonSettledinvalidation, which is simpler as afinallyblock.What changed
Commit 1 (foundation):
ai-utils.ts: NewbuildOrderedProfiles(profiles, profileOrder)pure function encapsulating the shared ordering logic.use-daemon-config.ts: ReturnsorderedProfilesas a derived slice (viauseMemo+buildOrderedProfiles).ai-types.ts:CallSiteOverrideDraftdefined here directly (no longer imported from modal). NewProfileWithNametype alias replaces the duplicateProfileinterface.call-site-overrides-modal.tsx: RemovedCallSiteOverrideDraftdefinition, usesorderedProfilesfrom hook.language-model-card.tsx: UsesorderedProfilesfrom hook instead of local derivation.manage-profiles-modal.tsx: RemovedProfileinterface andprofileEntryToProfilemapper (~35 lines deleted).profile-editor-modal.tsx: ImportsProfileWithNamefromai-types.Commit 2 (audit fixes):
manage-profiles-modal.tsx: PassesorderedProfilesfrom hook to inner component instead of recomputing locally.call-site-overrides-modal.tsx: Removed unnecessaryexportonCUSTOM_SENTINEL(only used within file).use-daemon-config.ts: EliminatedpatchConfigMutationandputImageGenModelMutationuseMutationhooks.patchDaemonConfigandsetImageGenModelOnDaemonnow call the SDK directly (configPatch,modelImagegenPut) withfinally-based cache invalidation viainvalidateConfig().Net: +22 −56 (audit) on top of +56 −101 (foundation) across 7 files. Pure refactor — no logic changes, no new behavior.
Follow-up tickets created
Larger architectural items identified during the audit, filed as sub-issues of LUM-2072:
useDaemonConfiggod-hook into query hook + separate mutation helpersRecord<string, unknown>)useEffecthydration with query-derived draft state in service cardsReferences
useMemoderived state pattern — derive from cache, don't copy{ name: string } & ProfileEntryreplaces manual field copyingPrompt / plan
LUM-2072 quick-wins phase — first-principles audit identified 5 issues. All fixed here; 3 larger items ticketed.
Test plan
bunx tsc --noEmit— cleanbun run lint— 0 errorsbun test src/domains/settings/ai/— 8/8 pass, 16 assertionsCloses LUM-2072
Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka